-
Notifications
You must be signed in to change notification settings - Fork 294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[persistent-mysql] Add support for conditional copying #693
Conversation
Sorry, but can you actually expand the use case you have explained with an example ? I'm having a difficult time following the problem this solves. Also in the code, you seem to have deleted the existing haddock comment for the type |
Sure thing! Let's say you've got a CSV row that maps to a database column. The CSV only ever contains changes to the value set, instead of the full values. {-
in a data definition file:
Row
productName Text
productPrice Double Maybe
productDescription Text Maybe
productQuantity Int Maybe
Primary productName
-}
csvRowHeader = "productName,productPrice,productDescription,productQuantity"
csvRowExample = "bestStuff,3.99,,3"
csvRowParsed :: Row
csvRowParsed = Row "bestStuff" (Just 3.99) Nothing (Just 3) If we did a bulk insert on these sorts of rows, then we'd be copying So instead, we can do
I feel like it better reflects what the type is actually for. I'd be happy to make it clearer or add examples. |
CopyUnlessEq :: PersistField typ => EntityField record typ -> typ -> SomeField record | ||
-- ^ Only copy the field if it is not equal to the provided value. | ||
|
||
-- | Copy the field into the database only if the value in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you bump the cabal version, update changelog and add the @since
haddock syntax for all the functions you have added here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing! Is adding a constructor like this a minor version bump?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to bump it to 2.6.2
. More info here: http://www.snoyman.com/blog/2017/06/how-to-send-me-a-pull-request
-- corresponding record is non-empty, where "empty" means the Monoid | ||
-- definition for 'mempty'. Useful for 'Text', 'String', 'ByteString', etc. | ||
copyUnlessEmpty :: (Monoid typ, PersistField typ) => EntityField record typ -> SomeField record | ||
copyUnlessEmpty field = CopyUnlessEq field mempty | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of the introduced functions are only seem to be applicable for insertManyOnDuplicateKeyUpdate
. Can you mention that in the haddock comments? Also can you add the example you mentioned here: #693 (comment) and explain it's usecase in a similar way we have done it here: persistent doc
Thanks for the explanation, that was helpful.
You mean update the existing row ? I have left my other comments in the code review. |
Yes, that's correct. I've added the documentation requested along with version bump. Thanks! |
The travis build is failing with
Any idea what the fix for that might be? |
About travis: the last successful run seems to have been on precise, but they have recently finished changing the default to trusty: https://blog.travis-ci.com/2017-08-31-trusty-as-default-status. I'm guessing we are suffering a trysty/mysql problem along the lines of travis-ci/travis-ci#8331. The images are being updated on Wednesday - https://docs.travis-ci.com/user/build-environment-updates/2017-09-06/ - so perhaps it will work then! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed this, and don't see any big problems, but I'm also not that familiar with either the code or the functionality in MySQL. I'd strongly recommend including some integration tests to ensure this is working as expected.
-- ^ Copy the field directly from the record. | ||
CopyUnlessEq :: PersistField typ => EntityField record typ -> typ -> SomeField record | ||
-- ^ Only copy the field if it is not equal to the provided value. | ||
-- /Since 2.6.2/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend using the @since 2.6.2
syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL, thanks!
I'll implement the documentation example as a test. |
I made a change to the implementation. I removed the I'm pretty sure I'm going to want to expand this to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I think this test change is wrong. The point about truncateTimeOfDay
is that most databases handle microseconds, so we just truncate any picoseconds. roundTime
truncates to whole seconds for a completely different purpose, namely as a hack for MySQL, and this has now become a load more complicated.
Good call, I didn't understand exactly what was going on there. I've reverted that change and used I can also revert the unrelated test changes and leave that for another PR, if you're OK with an unrelated red test. |
Well, MySQL passes, and SQLite is just dying for no reason now. |
I should leave the tests as they were, and we'll live with them failing for now. I'd like a little while to think about |
I've reverted the test changes. |
Is there anything I can do to help get this merged? :) |
OK by me - are you happy too @psibi ? We know why the test fails - for unrelated reasons - and I am addressing that separately. |
@paul-rouse LGTM. Please go ahead and merge. |
Thanks both of you! |
Added minor README enchancement.
This PR extends the
insertManyOnDuplicateKeyUpdate
function to allow conditional copying of data from the input record.Use case: Suppose you're importing a report that sometimes has partial data, and you want to copy the report into the database. The result record may have many
Nothing
fields. If we copy this using the currentSomeField
, then we will overwrite the existing data withNULL
s. Oops.The new constructor and functions permit us to avoid copying bad data, including
NULL
and empty strings, into the database, potentially overriding already present data.